Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive CLI interface to the sanpy library built with Typer. It adds configuration management for API keys, commands for metrics and projects discovery, data fetching and rate limit monitoring, multiple output formatters (JSON, CSV, table), and extensive unit and integration tests. A minor notebook import cleanup is also included. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Command Handler
participant Config as Config Module
participant API as Santiment API
participant Formatter as Output Formatter
User->>CLI: Execute command (e.g., get metric)
CLI->>Config: Get stored or env API key
Config-->>CLI: Return API key
CLI->>API: Make API request with key
API-->>CLI: Return data (DataFrame/dict)
CLI->>Formatter: Format data (json/csv/table)
Formatter-->>CLI: Formatted output
CLI->>User: Display output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@san/tests/test_cli_integration.py`:
- Around line 130-131: The test currently uses a weak check `assert "datetime"
in data[0] or "value" in data[0]`; update the assertion to require both fields
by changing it to `assert "datetime" in data[0] and "value" in data[0]` (i.e.,
validate that both keys exist on the first element of `data`), so the structure
check for `data[0]` verifies both expected fields are present.
In `@san/tests/test_cli.py`:
- Around line 24-29: The fixture mock_api_key is unused; either remove the
unused pytest fixture definition named mock_api_key or attach it to tests that
require it by adding mock_api_key as a test parameter; if keeping it, ensure the
patch target "san.ApiConfig" is correct and referenced by tests (e.g., include
mock_api_key in test function signatures that rely on san.ApiConfig.api_key)
otherwise delete the entire mock_api_key fixture block to eliminate dead code.
🧹 Nitpick comments (8)
san/cli_config.py (2)
24-32: Potential type inconsistency withPathconversion.When
APPDATAorXDG_CONFIG_HOMEis not set,Path.home()returns aPathobject, but when the env var exists,os.environ.get()returns a string. WhilePath(base)on line 31 handles this, it would be cleaner to ensure consistent types.♻️ Suggested improvement
def get_config_dir() -> Path: """Get the platform-appropriate config directory.""" if os.name == "nt": # Windows - base = os.environ.get("APPDATA", Path.home()) + base = Path(os.environ.get("APPDATA", str(Path.home()))) else: # Linux/Mac - base = os.environ.get("XDG_CONFIG_HOME", Path.home() / ".config") + xdg_config = os.environ.get("XDG_CONFIG_HOME") + base = Path(xdg_config) if xdg_config else Path.home() / ".config" - config_dir = Path(base) / "sanpy" + config_dir = base / "sanpy" return config_dir
56-65: Silent error handling may hide config file corruption.When
_load_configencounters aJSONDecodeError, it silently returns an empty dict. This could mask a corrupted config file from the user. Consider logging a warning or informing the user that their config was reset.♻️ Suggested improvement
def _load_config() -> dict: - """Load config from file, return empty dict if doesn't exist.""" + """Load config from file, return empty dict if doesn't exist or is corrupted.""" config_path = get_config_path() if config_path.exists(): try: with open(config_path, "r") as f: return json.load(f) - except (json.JSONDecodeError, IOError): + except json.JSONDecodeError: + # Config file is corrupted, return empty and let next save overwrite + import sys + print(f"Warning: Config file '{config_path}' is corrupted, using defaults.", file=sys.stderr) + return {} + except IOError: return {} return {}san/tests/test_cli_integration.py (1)
321-338: Consider adding assertion for error message content.The test verifies the exit code and that "error" appears in stdout, but doesn't validate that the error is specifically about the invalid slug. This could help catch regressions in error messaging.
san/cli_formatters.py (2)
49-54: CSV and table formats produce identical output for lists.The
csvandtablebranches produce the same output ("\n".join(items)). For true CSV format, consider adding a header or quoting values. If this is intentional, a comment would clarify the design choice.
100-104: Inconsistent CSV escaping - consider using csv module.
format_dictuses thecsvmodule for proper escaping, butformat_api_callsuses string formatting. If a date string contains commas or special characters, this could produce malformed CSV.♻️ Suggested fix
elif fmt == "csv": - lines = ["date,api_calls"] - for date, count in calls: - lines.append(f"{date},{count}") - return "\n".join(lines) + buffer = io.StringIO() + writer = csv.writer(buffer, quoting=csv.QUOTE_MINIMAL) + writer.writerow(["date", "api_calls"]) + for date, count in calls: + writer.writerow([date, count]) + return buffer.getvalue()san/cli.py (2)
72-78: Consider security implications of displaying config path.Echoing the full config path after saving the API key is helpful for transparency, but ensure users understand the key is stored in plaintext.
♻️ Optional enhancement for security awareness
def config_set_key( api_key: Annotated[str, typer.Argument(help="Your Santiment API key")], ) -> None: """Store API key in config file.""" set_api_key(api_key) - output(f"API key saved to {get_config_path()}") + output(f"API key saved to {get_config_path()}") + output("Note: The API key is stored in plaintext.")
164-188:--slugis a required option - consider making it a positional argument.The
slugparameter is marked as required (no default value), which is correct. However, for better UX, consider whetherslugshould be a positional argument likemetric, enablingsan get price_usd bitcoininstead ofsan get price_usd --slug bitcoin.setup.py (1)
19-19: Update the typer version constraint.The
typer>=0.9.0constraint is significantly outdated. The latest stable version is0.21.1(released Jan 6, 2026). Consider updating the constraint totyper>=0.21.0or higher to benefit from bug fixes, performance improvements, and new features from recent releases.
| # Check structure - should have datetime and value | ||
| assert "datetime" in data[0] or "value" in data[0] |
There was a problem hiding this comment.
Weak assertion - consider checking for both fields.
The assertion "datetime" in data[0] or "value" in data[0] will pass if only one field exists. This should verify that both expected fields are present.
💚 Suggested fix
- # Check structure - should have datetime and value
- assert "datetime" in data[0] or "value" in data[0]
+ # Check structure - should have datetime and value
+ assert "datetime" in data[0], "Expected 'datetime' field in JSON output"
+ assert "value" in data[0], "Expected 'value' field in JSON output"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check structure - should have datetime and value | |
| assert "datetime" in data[0] or "value" in data[0] | |
| # Check structure - should have datetime and value | |
| assert "datetime" in data[0], "Expected 'datetime' field in JSON output" | |
| assert "value" in data[0], "Expected 'value' field in JSON output" |
🤖 Prompt for AI Agents
In `@san/tests/test_cli_integration.py` around lines 130 - 131, The test currently
uses a weak check `assert "datetime" in data[0] or "value" in data[0]`; update
the assertion to require both fields by changing it to `assert "datetime" in
data[0] and "value" in data[0]` (i.e., validate that both keys exist on the
first element of `data`), so the structure check for `data[0]` verifies both
expected fields are present.
| @pytest.fixture | ||
| def mock_api_key(): | ||
| """Set up a mock API key for tests.""" | ||
| with patch("san.ApiConfig") as mock_config: | ||
| mock_config.api_key = "test_api_key_12345" | ||
| yield mock_config |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check imports and usage in test_cli.py
echo "=== Imports and fixture usage in test_cli.py ==="
head -40 san/tests/test_cli.py
echo ""
echo "=== ApiConfig imports and usage in cli.py ==="
rg -n "ApiConfig|from san|import san" san/cli.py | head -20
echo ""
echo "=== ApiConfig definition ==="
rg -n "class ApiConfig" san/
echo ""
echo "=== How mock_api_key fixture is used in tests ==="
rg -n "mock_api_key" san/tests/test_cli.py | head -10Repository: santiment/sanpy
Length of output: 1505
🏁 Script executed:
#!/bin/bash
# Find tests that use mock_api_key parameter
echo "=== Tests using mock_api_key fixture ==="
rg -B2 "def test.*mock_api_key" san/tests/test_cli.py
echo ""
echo "=== Check if mock_api_key is imported from anywhere ==="
rg "from.*mock_api_key|import.*mock_api_key" san/tests/test_cli.py
echo ""
echo "=== Show full imports section of cli.py to understand san module ==="
head -30 san/cli.py
echo ""
echo "=== Check what san module is ==="
cat san/__init__.pyRepository: santiment/sanpy
Length of output: 2412
🏁 Script executed:
#!/bin/bash
# Search for all test function definitions
echo "=== All test functions in test_cli.py ==="
rg "^def test_" san/tests/test_cli.py | head -20
echo ""
echo "=== Check entire fixture section and how fixtures are used ==="
rg -A30 "@pytest.fixture" san/tests/test_cli.py
echo ""
echo "=== Check how mock_api_key is actually used in tests ==="
rg -C5 "mock_api_key" san/tests/test_cli.pyRepository: santiment/sanpy
Length of output: 2738
🏁 Script executed:
#!/bin/bash
# Search more carefully - maybe fixture is used but with autouse or other method
echo "=== Check if mock_api_key fixture has autouse ==="
grep -n "autouse\|mock_api_key" san/tests/test_cli.py
echo ""
echo "=== Count total lines to see complete file ==="
wc -l san/tests/test_cli.py
echo ""
echo "=== Check the entire fixtures section and beyond ==="
sed -n '24,100p' san/tests/test_cli.pyRepository: santiment/sanpy
Length of output: 2492
🏁 Script executed:
#!/bin/bash
# Check conftest.py files
echo "=== Look for conftest.py files ==="
find san/tests -name "conftest.py" -exec cat {} \;
echo ""
echo "=== Check if mock_api_key is referenced anywhere else in tests ==="
rg "mock_api_key" san/tests/
echo ""
echo "=== Let's verify the patch mechanism by checking mock usage in existing tests ==="
sed -n '100,150p' san/tests/test_cli.pyRepository: santiment/sanpy
Length of output: 2228
🏁 Script executed:
#!/bin/bash
# Let's verify the exact import structure and how patching works
echo "=== Verify san module structure ==="
python3 << 'PYTHON'
import sys
sys.path.insert(0, '/tmp/repo')
# Simulate the imports as they happen in cli.py
import san
print(f"san.ApiConfig: {san.ApiConfig}")
print(f"san.ApiConfig is from san.api_config: {san.ApiConfig.__module__}")
# Verify that patching at "san.ApiConfig" would work
from unittest.mock import patch
# This is what the fixture does
with patch("san.ApiConfig") as mock_config:
mock_config.api_key = "test_key"
# Now when code does import san; san.ApiConfig.api_key
# Would it be affected?
print(f"Inside patch context: san.ApiConfig = {san.ApiConfig}")
print(f"san.ApiConfig.api_key = {san.ApiConfig.api_key}")
PYTHON
echo ""
echo "=== Verify the actual test runs and behavior ==="
cd /tmp/repo && python3 -m pytest san/tests/test_cli.py::test_version -v 2>&1 | head -30Repository: santiment/sanpy
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Simpler approach - analyze the code structure without running tests
echo "=== Verify fixture is not used by any test ==="
rg "def test_.*\(.*mock_api_key" san/tests/test_cli.py
echo ""
echo "=== Check how other fixtures are used (with actual parameters) ==="
grep -n "def test_.*(" san/tests/test_cli.py | head -10
echo ""
echo "=== Analyze the patch target logic ==="
echo "Looking at imports in test_cli.py:"
head -15 san/tests/test_cli.py
echo ""
echo "Looking at patch in fixture:"
sed -n '24,29p' san/tests/test_cli.py
echo ""
echo "=== Show how other mocks patch things ==="
grep -n "@patch" san/tests/test_cli.py | head -10
echo ""
echo "=== Example of `@patch` usage in the file ==="
sed -n '138,145p' san/tests/test_cli.pyRepository: santiment/sanpy
Length of output: 1768
🏁 Script executed:
#!/bin/bash
# Final verification: is the fixture genuinely unused?
echo "=== Search for mock_api_key in entire test directory ==="
rg "mock_api_key" san/tests/
echo ""
echo "=== Check git history to see if this fixture was recently added/removed usage ==="
git log --oneline -n 20 san/tests/test_cli.py 2>/dev/null || echo "Git not available"
echo ""
echo "=== Verify patch target comprehensively ==="
echo "When cli.py does 'import san', it gets san module"
echo "san/__init__.py imports ApiConfig like this:"
grep "from.*ApiConfig\|import.*ApiConfig" san/__init__.py
echo ""
echo "So cli.py accesses it as san.ApiConfig, which means patching 'san.ApiConfig' is correct."
echo ""
echo "Let's verify the other `@patch` decorators use the same pattern:"
grep "@patch" san/tests/test_cli.py | head -5Repository: santiment/sanpy
Length of output: 737
Fixture is unused and should be removed.
The mock_api_key fixture is defined but never referenced by any test—no test function includes it as a parameter. Since it's not being used, either add it to test functions that need it (e.g., by including mock_api_key as a parameter) or remove it entirely as dead code.
Note: The patch target "san.ApiConfig" is actually correct, as cli.py imports the san module and accesses san.ApiConfig.api_key.
🤖 Prompt for AI Agents
In `@san/tests/test_cli.py` around lines 24 - 29, The fixture mock_api_key is
unused; either remove the unused pytest fixture definition named mock_api_key or
attach it to tests that require it by adding mock_api_key as a test parameter;
if keeping it, ensure the patch target "san.ApiConfig" is correct and referenced
by tests (e.g., include mock_api_key in test function signatures that rely on
san.ApiConfig.api_key) otherwise delete the entire mock_api_key fixture block to
eliminate dead code.
1 participant
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.